Skip to content

Added logic for working with Tarantool schema via Box #426

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KaymeKaydex
Copy link

@KaymeKaydex KaymeKaydex commented Dec 27, 2024

  • Implemented the box.Schema() method that returns a Schema object for schema-related operations

What has been done? Why? What problem is being solved?

I didn't forget about (remove if it is not applicable):

Related issues:

@KaymeKaydex KaymeKaydex force-pushed the box/schema branch 2 times, most recently from 2e271da to 9f736f4 Compare December 27, 2024 17:51
Copy link
Collaborator

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, add more unit-tests for a new requests/response. You could see how it is tested on the main package and inside the crud subpackage.

We also need examples (in example_test.go) with new features. This will help users to use the library.

Finally, we need to keep a backward compatibility until a next major release, so we need to add another Info() method with a context instead of changing the current one.

@KaymeKaydex KaymeKaydex force-pushed the box/schema branch 11 times, most recently from 5c9bb77 to 1e3625c Compare January 21, 2025 15:15
@KaymeKaydex
Copy link
Author

Please, add more unit-tests for a new requests/response. You could see how it is tested on the main package and inside the crud subpackage.

We also need examples (in example_test.go) with new features. This will help users to use the library.

Finally, we need to keep a backward compatibility until a next major release, so we need to add another Info() method with a context instead of changing the current one.

i removed interface changes
added new examples for new methods
and added more tests to make coverage up, now all methods are covered

@KaymeKaydex
Copy link
Author

Please, fix the commit message:

box: ddded logic for working with Tarantool schema

Implemented the `box.Schema()` method that returns a `Schema`
object for schema-related operations

done

@KaymeKaydex KaymeKaydex force-pushed the box/schema branch 2 times, most recently from 40555ec to 4954fc7 Compare January 30, 2025 09:19
@KaymeKaydex KaymeKaydex closed this Feb 5, 2025
@KaymeKaydex KaymeKaydex reopened this Mar 14, 2025
@KaymeKaydex KaymeKaydex force-pushed the box/schema branch 9 times, most recently from 3695443 to 5d690db Compare April 16, 2025 09:56
Implemented the `box.Schema()` method that returns a `Schema`
object for schema-related operations
@KaymeKaydex
Copy link
Author

@oleg-jukovec oleg-jukovec requested review from dmyger and removed request for DerekBum and themilchenko May 25, 2025 17:48
Copy link
Collaborator

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch. It would be nice if @dmyger paid attention to the test per function coverage.

As I could see the code is not covered (and some MessagePack unmarshal errors, but it is not critical):
image

Please, add a test for it.

Comment on lines +20 to +24
// Schema returns a new Schema instance, providing access to schema-related operations.
// It uses the connection from the Box instance to communicate with Tarantool.
func (b *Box) Schema() *Schema {
return NewSchema(b.conn)
}
Copy link
Collaborator

@oleg-jukovec oleg-jukovec May 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to write a unit-test that checks whether the created circuit uses the connection passed to box.New.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants